-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[android] Don't include JNI_OnLoad in libSystem.Security.Cryptography.Native.Android.a
#103231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[android] Don't include JNI_OnLoad in libSystem.Security.Cryptography.Native.Android.a
#103231
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
|
|
||
| JNIEXPORT jint JNICALL | ||
| JNI_OnLoad(JavaVM *vm, void *reserved) | ||
| jint init_library_on_load (JavaVM *vm, void *reserved) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this something else that includes the lib name so we don't end up with duplicate init_library_on_load if we add JNI_OnLoad to another library.
Something like AndroidCryptoNative_InitLibraryOnLoad would fit the existing naming convention better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also mark it as PALEXPORT in the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be exported. It's called by either android crypto's JNI_OnLoad when building a dynamic library or it will be called by our Android for .NET's one if we link it in statically. In neither case it needs to be exported, it's an internal call.
|
/cc @vitek-karas |
| jint AndroidCryptoNative_InitLibraryOnLoad (JavaVM *vm, void *reserved) | ||
| { | ||
| (void)reserved; | ||
| LOG_INFO("JNI_OnLoad in pal_jni.c"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this outdated log message might be confusing
| LOG_INFO("JNI_OnLoad in pal_jni.c"); | |
| LOG_INFO("AndroidCryptoNative_InitLibraryOnLoad in pal_jni.c"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually planned on removing the message in a separate PR. It serves no real purpose and logcat calls are very expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has sometimes been useful in the past but LOG_DEBUG should be enough
src/native/libs/System.Security.Cryptography.Native.Android/pal_jni_onload.c
Show resolved
Hide resolved
|
@steveisok do you remember if the LibraryBuilder uses the static libs for Android? If yes we'll also need to fix it there. |
…phy.Native.Android.a` I'm working on a feature in .NET For Android which will allow us to link all the native bits into a single shared library at application build time. In order to make it possible, no archive (`.a`) may contain any `JNI_OnLoad` functions (called by `JavaVM` when initializing a Java extension DSO), because our runtime already contains one and there Can be Only One(tm). `libSystem.Security.Cryptography.Native.Android` is currently the only BCL support native library which contains `JNI_OnLoad` and thus it prevents us from linking it into our runtime. This PR changes things a bit my moving the initialization code to a separate function (`init_library_on_load`) which remains in the `.a` archive and can be called by `.NET For Android` runtime from its own `JNI_OnLoad` as well as by the `libSystem.Security.Cryptography.Native.Android.so` from its `JNI_OnLoad`, which this PR moves to a separate source file that is compiled only into the shared version of the crypto support library.
…l_jni_onload.c Co-authored-by: Alexander Köplinger <[email protected]>
af4b8bf to
e212ed4
Compare
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I checked LibraryBuilder and it seems to use only the shared libraries so this is good to go. |
…ryptography.Native.Android.a` (dotnet#103231)" This reverts commit 048c8ed.
I'm working on a feature in .NET For Android which will allow us to link
all the native bits into a single shared library at application build
time. In order to make it possible, no archive (
.a) may contain anyJNI_OnLoadfunctions (called byJavaVMwhen initializing a Javaextension DSO), because our runtime already contains one and there Can
be Only One(tm).
libSystem.Security.Cryptography.Native.Androidis currently the onlyBCL support native library which contains
JNI_OnLoadand thus itprevents us from linking it into our runtime. This PR changes things
a bit my moving the initialization code to a separate
function (
AndroidCryptoNative_InitLibraryOnLoad) which remains in the.aarchive andcan be called by
.NET For Androidruntime from its ownJNI_OnLoadaswell as by the
libSystem.Security.Cryptography.Native.Android.sofromits
JNI_OnLoad, which this PR moves to a separate source file that iscompiled only into the shared version of the crypto support library.